Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: updated /cc list for inspector issues #13632

Closed
wants to merge 2 commits into from
Closed

doc: updated /cc list for inspector issues #13632

wants to merge 2 commits into from

Conversation

thelostone-mc
Copy link
Contributor

@thelostone-mc thelostone-mc commented Jun 12, 2017 •
edited
Loading

Updated onboarding-extras.md to /cc @nodejs/v8-inspector
for inspector issues and reorganized the list alphabetically

Fixes: #13621

Files changes
  • doc/onboarding-extras.md
Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Jun 12, 2017

Can you use the full URL in the Fixes line, so:

Fixes: https://github.com/nodejs/node/issues/13621

?

gibfahn
gibfahn approved these changes Jun 12, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

@thelostone-mc
Copy link
Contributor Author

thelostone-mc commented Jun 12, 2017 •
edited
Loading

@gibfahn Done!!
By the way does it have any significant difference ?

@gibfahn
Copy link
Member

gibfahn commented Jun 12, 2017

By the way does it have any significant difference ?

Yes! It's not obvious, but we moved our repo from nodejs/node-v0.x-archive to nodejs/node, and so any links like #13621 redirect to the wrong place. Using an absolute link means that (in theory) it will work even if we had to move the repo again.

Also it helps people who aren't using Github (who just want a link to copy/paste in).

@thelostone-mc
Copy link
Contributor Author

@gibfahn Ah alright!! Makes sense to me !
Absolute url all the way 👍

@aqrln
Copy link
Contributor

aqrln commented Jun 12, 2017

It might be worth to move this entry above in the list and make it something like "src/inspector_*, lib/inspector.js" instead of "inspector issues" to be consistent with other subsystems.

@thelostone-mc
Copy link
Contributor Author

thelostone-mc commented Jun 12, 2017 •
edited
Loading

@aqrln actually makes sense! Not sure why I didn't do that in the first place -.-
@gibfahn Would it be alright if I changed it ? Avoids inconsistency

@gibfahn
Copy link
Member

gibfahn commented Jun 12, 2017

It might be worth to move this entry above in the list and make it something like "src/inspector_*, lib/inspector.js" instead of "inspector issues" to be consistent with other subsystems.

SGTM

Would it be alright if I changed it ?

Yep, seems like a good idea to me.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Jun 12, 2017
Trott
Trott previously requested changes Jun 13, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except that I think the new information should be moved up by two lines.

doc/onboarding-extras.md Outdated
@@ -25,6 +25,7 @@
| `src/async-wrap.*` | @nodejs/async_hooks |
| `src/node_api.*` | @nodejs/n-api |
| `src/node_crypto.*` | @nodejs/crypto |
| `src/inspector_*`, `lib/inspector.js` | @nodejs/v8-inspector |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this up two lines so all the directories are still in alphabetical order?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking more closely, it might make sense to change it so lib/inspector.js comes first in the line befor src/inspector_*. That would be consistent with elsewhere in the doc. (I guess everything defaults alphabeitcal?) And then move the line to the corresponding location in the list.

Sorry for the nit-picking. Docs are like that. :-|

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, there are other places where the alphabetical stuff is messed up, but it's mostly alphabetical and perhaps a second good commit would be to make it all alphabetical. But let's get this one line added first...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott Ah totally makes sense!! Done. Added a second commit to. Is that alright ?

@thelostone-mc
doc: updated /cc list for inspector issues
c3fbe02 
Updated onboarding-extras.md to /cc @nodejs/v8-inspector
for inspector issues.

Fixes: nodejs#13621
@thelostone-mc
doc: reorganized /cc list in alphabetical order
44f2752 
reorganized /cc list in alphabetical order in
onboarding-extras.md

Refs: nodejs#13621
@Trott Trott dismissed their stale review June 13, 2017 17:14

Looks like my comment has been addressed. Dismissing review. Thanks for the contribution! 🎉

tniessen pushed a commit to tniessen/node that referenced this pull request Jun 14, 2017
@thelostone-mc @tniessen
doc: sort and update /cc list for inspector issues
3d21569 
Updated onboarding-extras.md to /cc @nodejs/v8-inspector for inspector
issues and reorganized /cc list in alphabetical order.

PR-URL: nodejs#13632
Fixes: nodejs#13621
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@tniessen
Copy link
Member

Landed in 3d21569, thank you for your contribution 🎉

@tniessen tniessen closed this Jun 14, 2017
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@thelostone-mc @addaleax
doc: sort and update /cc list for inspector issues
8c15827 
Updated onboarding-extras.md to /cc @nodejs/v8-inspector for inspector
issues and reorganized /cc list in alphabetical order.

PR-URL: #13632
Fixes: #13621
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@addaleax addaleax mentioned this pull request Jun 17, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@thelostone-mc @addaleax
doc: sort and update /cc list for inspector issues
f5f2a0e 
Updated onboarding-extras.md to /cc @nodejs/v8-inspector for inspector
issues and reorganized /cc list in alphabetical order.

PR-URL: #13632
Fixes: #13621
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@gibfahn gibfahn gibfahn approved these changes

@aqrln aqrln aqrln approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@eugeneo eugeneo eugeneo approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

entry for inspector in onboarding-extras.md